Bind fan-out lineage vars in instance middleware#189
Merged
Conversation
current_fan_out_index() (and the lineage chains) returned None inside fan-out instance_middleware: the engine binds those ContextVars per-node inside the inner subgraph (compiled.py), but instance_middleware wraps the subgraph from outside, before any node runs. The documented use (RetryMiddleware) doesn't read the index, so it sat latent; custom instance middleware reading the index or calling set_invocation_metadata saw None. Bind the three fan-out lineage ContextVars (fan_out_index + the per-depth index/branch chains) to the instance's child_context around the instance_middleware chain via a _bind_instance_lineage context manager, resetting on exit. Bind only when there is instance middleware to read them (the inner nodes bind them otherwise), so the no-middleware path is unchanged. Reset before the error-handling below so its saves keep their existing context.
There was a problem hiding this comment.
Pull request overview
This PR fixes fan-out instance_middleware seeing unset fan-out lineage ContextVars by binding the instance’s lineage (fan_out_index, index chain, branch chain) around the instance-middleware chain in fan_out.py, matching the per-node binding already done deeper in execution.
Changes:
- Add
_bind_instance_lineage(...)context manager and apply it around theinstance_middlewarechain execution in fan-out instances. - Add unit tests asserting
instance_middlewarecan readcurrent_fan_out_index()/current_fan_out_index_chain()and that lineage is reset on failure. - Document the fix in
CHANGELOG.mdunder Unreleased.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/openarmature/graph/fan_out.py |
Bind/reset fan-out lineage ContextVars around instance_middleware execution for each fan-out instance. |
tests/unit/test_fan_out.py |
Add regression tests for index visibility inside instance middleware and reset behavior on exceptions. |
CHANGELOG.md |
Add Unreleased “Fixed” entry documenting the behavior correction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
From CoPilot review of #189: expand the two new instance-middleware tests' inner and parent graph construction from inline method chains to the step-by-step named-builder pattern used throughout the module.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
current_fan_out_index()(andcurrent_fan_out_index_chain()) returnedNoneinside fan-outinstance_middleware, even though the middleware is logically running inside a specific instance.Root cause
The engine binds the fan-out lineage ContextVars per-node, inside the inner subgraph (
compiled.py). Butinstance_middlewarewraps the inner subgraph from the outside (fan_out.py), before any node runs, so the vars are still unset there. Node-level reads (node bodies, node middleware like failure-isolation, the provider) all work; only instance-level middleware sawNone.instance_middlewareis a public, documented extension point (theadd_fan_out_nodesignature;docs/concepts/middleware.md; the fan-out-with-retry example). Its documented use,RetryMiddleware, does not read the index, which is why this sat latent. Custom instance middleware that reads the index or callsset_invocation_metadatawould seeNone.Change
Bind the three lineage ContextVars (
fan_out_indexplus the per-depth index/branch chains) to the instance'schild_contextaround theinstance_middlewarechain, via a_bind_instance_lineagecontext manager, resetting on exit. Mirrors whatcompiled.pyalready does per-node.set_invocation_metadata's depth-aligned lineage view stays consistent.No shipped behavior change: nothing in the codebase reads the index from instance middleware, and no fixture pins the current
None.Test plan
tests/unit/test_fan_out.py:test_instance_middleware_sees_fan_out_index(the index + chain are the instance's, per instance) andtest_instance_middleware_lineage_reset_on_failure(the binding resets on the exception path). Both fail without the fix.tests/-> 1462 passed. ruff + pyright clean.CHANGELOG entry under Unreleased (rides into v0.16.0).